feat: TipRouter CLI separate stages #87
Conversation
This reverts commit 22c7d6a.
There was a problem hiding this comment.
Can we add something that will purge the saved files in save_path that are older than “NUM_MONITORED_EPOCHS”? And should make sure that NUM_MONITORED_EPOCHS > 0.
Then we should add something in the release notes that the saved file formats are changing from “file_name_{epoch}.json” to “{epoch}_file_name.json” and will be auto purged, so any files that operators have in the old format they should manually remove
| some_stake_meta_collection, | ||
| epoch_to_process, | ||
| ncn_address, | ||
| PROTOCOL_FEE_BPS, |
There was a problem hiding this comment.
Oops this was an oversight.
Based on context from generator where these fees are actually deducted from tip_distribution_meta.total_tips I'm pretty sure it should be total_fee_bps() (like the following). But want to confirm
let config = get_ncn_config(&rpc_client, &tip_router_program_id, &ncn_address).await?;
let current_fees = config.fee_config.current_fees(epoch);
let protocol_fee_bps = current_fees.total_fees_bps()?
There was a problem hiding this comment.
it'd be what we have now: adjusted_total_fees_bps()
https://github.com/jito-foundation/jito-tip-router/blob/master/tip-router-operator-cli/src/process_epoch.rs#L117
There was a problem hiding this comment.
Updated: a1c33dc
There was a problem hiding this comment.
epoch should be:
tip_router_target_epoch = epoch + 1
Confusing convention but:
"target epoch" refers to the epoch that the snapshot is being created for (so usually the previous epoch)
"tip router target epoch" refers to the epoch after that which the on-chain program uses for that data
E.g. right now is epoch 10
"target epoch" is epoch 9, so we get 9_meta_merkle_tree.json, etc
But the ballot box, etc on chain for this meta_merkle_root will be in BallotBox::find_program_address(ncn, 10)
There was a problem hiding this comment.
ahhh that is confusing and great to know! Updated: 74b252a
| // TODO: REVIEW should we expect snapshots to be in the save path rather than | ||
| // the typical validator snapshots path? This would save the fight from the | ||
| // validator process removing snapshots. We'd have to also update the snapshot | ||
| // process and CLI to handle |
There was a problem hiding this comment.
Yes we want to stop fighting the validator purging snapshots so save path is the best option - alternatively could we do something like: change the OperatorState enum to OperatorState::CreateStakeMeta(Option) where the inner value is the full snapshot path, if no bank is passed in? so operators would technically have a choice
There was a problem hiding this comment.
Variant with data causes clap::ValueEnum derive macro to fail. I think a separate CLI argument may make the most sense.
What I've determined is anywhere load_bank_from_snapshot is called, we assume the user is trying to save a new snapshot (or load the bank without having the snapshot). So in this case we should be using the existing full snapshots to load the bank.
Then where get_bank_from_snapshot_at_slot is used, we expect the snapshot at the exact slot to already exist. So here we should use the save-path as the full snapshot path. Which holds true if we save the full snapshots at save-path during creation.
There was a problem hiding this comment.
I'll get my proposed changes up for review shortly
There was a problem hiding this comment.
There was a problem hiding this comment.
seeing some oddities with some snpashot file placement with this. Looking into it. Everything else seemed to be running fine at the last epoch boundary
There was a problem hiding this comment.
nice! that logic makes sense for the variant issue
| starting_stage: OperatorState, | ||
|
|
||
| #[arg(long, env, default_value = "true")] | ||
| save_stages: bool, |
There was a problem hiding this comment.
could we have one other for save_snapshot: bool that defaults to false just because the snapshots are so much bigger than the other stages
There was a problem hiding this comment.
Renamed enable_snapshots => save_snapshot since that feature flag already existed and added an alias
Change notes
META_MERKLE_TREE_DIR. Path where all generated operator files are storedrunCLI command loops through each stage, saving the outputs to save-path CLI argument. The output from each stage is held in memory to reduce the processing.runload-bank-from-snapshot, create-stake-meta, create-merkle-tree-collection, create-meta-merkle-tree, cast-vote, wait-for-next-epochStakeMetaCollectionfile which contains validator vote accounts and their staking delegations.StakeMetaCollectionfrom disk at save-path, generate and save theGeneratedMerkleTreeCollection.GeneratedMerkleTreeCollectionfrom disk at save-path, generate and save theMetaMerkleTree.